Delay short-circuit in dry-run.rb if vulnerable#6258
Merged
jeffwidman merged 1 commit intomainfrom Dec 5, 2022
Merged
Conversation
This was referenced Dec 4, 2022
vulnerabledry-run.rb if vulnerable
landongrindheim
approved these changes
Dec 5, 2022
Nishnha
approved these changes
Dec 5, 2022
32c269b to
52195d9
Compare
In #5950, this short-circuit was moved up to skip checking for the latest version if we already know we're on the latest version. During review, it was moved below the case for a security update, when a dep is already up-to-date but also _not_ vulnerable. However, there's yet another case, which is where a dep is up-to-date and _also_ vulnerable. I was surprised in #6230 (comment) to see that it spits out `no update needed as it's already up-to-date` when I had explicitly passed in a security advisor marking the latest version as vulnerable. It wasn't clear to me if the `dry-run` was prematurely bailing before checking for vulnerable versions. That was for the `pub` ecosystem, where both `checker.lowest_resolvable_security_fix_version` and `checker.latest_resolvable_version` return the same version. The very next `if` block handles the case when the dep _is_ vulnerable. So by moving this short-circuit below that, it will still short circuit for the common case where a version is not vulnerable and on latest. For reference, here's pre-change run: ``` [dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2. Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. => reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev => parsing dependency files I, [2022-12-04T22:26:35.101963 #38484] INFO -- : Failed to infer the flutter version. Attempting to use latest stable release. I, [2022-12-04T22:26:35.102107 #38484] INFO -- : Cloning the flutter repo https://github.com/flutter/flutter. I, [2022-12-04T22:27:40.833227 #38484] INFO -- : Checking out Flutter version stable I, [2022-12-04T22:27:46.471869 #38484] INFO -- : Running `flutter doctor` to install artifacts and create flutter/version. I, [2022-12-04T22:29:39.504293 #38484] INFO -- : Running `flutter --version` I, [2022-12-04T22:29:40.773354 #38484] INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5. => updating 1 dependencies: buffer === buffer (1.1.1) (vulnerable 🚨) => checking for updates 1/1 I, [2022-12-04T22:29:41.511587 #38484] INFO -- : Failed to infer the flutter version. Attempting to use latest stable release. I, [2022-12-04T22:29:41.511704 #38484] INFO -- : Checking out Flutter version stable I, [2022-12-04T22:29:42.735602 #38484] INFO -- : Running `flutter doctor` to install artifacts and create flutter/version. I, [2022-12-04T22:29:45.652134 #38484] INFO -- : Running `flutter --version` I, [2022-12-04T22:29:46.743673 #38484] INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5. => latest available version is 1.1.1 (no update needed as it's already up-to-date) 🌍 Total requests made: '0' ``` And here's post-change run: ``` [dependabot-core-dev] ~/dependabot-core $ SECURITY_ADVISORIES='[{"dependency-name":"buffer","patched-versions":[],"unaffected-versions":[],"affected-versions":["<= 2.0.0"]}]' bin/dry-run.rb pub dart-lang/pub-dev --dir "/app" --cache=files --dep="buffer" # direct dep warning: parser/current is loading parser/ruby31, which recognizes3.1.3-compliant syntax, but you are running 3.1.2. Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri. => reading cloned repo from /home/dependabot/dependabot-core/tmp/dart-lang/pub-dev => parsing dependency files I, [2022-12-04T22:44:40.544797 #39357] INFO -- : Failed to infer the flutter version. Attempting to use latest stable release. I, [2022-12-04T22:44:40.544922 #39357] INFO -- : Checking out Flutter version stable I, [2022-12-04T22:44:41.954572 #39357] INFO -- : Running `flutter doctor` to install artifacts and create flutter/version. I, [2022-12-04T22:44:45.209362 #39357] INFO -- : Running `flutter --version` I, [2022-12-04T22:44:46.159503 #39357] INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5. => updating 1 dependencies: buffer === buffer (1.1.1) (vulnerable 🚨) => checking for updates 1/1 I, [2022-12-04T22:44:46.554930 #39357] INFO -- : Failed to infer the flutter version. Attempting to use latest stable release. I, [2022-12-04T22:44:46.555046 #39357] INFO -- : Checking out Flutter version stable I, [2022-12-04T22:44:47.709661 #39357] INFO -- : Running `flutter doctor` to install artifacts and create flutter/version. I, [2022-12-04T22:44:51.640507 #39357] INFO -- : Running `flutter --version` I, [2022-12-04T22:44:52.616051 #39357] INFO -- : Installed the Flutter SDK version: 3.3.9 with Dart 2.18.5. => latest available version is 1.1.1 => there is no available non-vulnerable version (no update needed as it's already up-to-date) 🌍 Total requests made: '0' ``` Note that the output above is slightly confusing because it's _not_ doing a security update only, but rather a normal update with a specified security advisory... so I think it's fine for debugging purposes that we get the information that there's no non-vulnerable version, and also that from a version-checking perspective there's no available update, regardless of vulnerability.
52195d9 to
a0909c7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #5950, this short-circuit was moved up to skip checking for the latest version if we already know we're on the latest version. (Which is awesome! It's a great speedup)
During review, it was moved below the case for a security update, when a dep is already up-to-date but also not vulnerable.
However, there's yet another case, which is where a dep is up-to-date and also vulnerable. I was surprised in
#6230 (comment) to see that it spits out
no update needed as it's already up-to-datewhen I had explicitly passed in a security advisor marking the latest version as vulnerable. It wasn't clear to me if thedry-runwas prematurely bailing before checking for vulnerable versions. That was for thepubecosystem, where bothchecker.lowest_resolvable_security_fix_versionandchecker.latest_resolvable_versionreturn the same version.The very next
ifblock handles the case when the dep is vulnerable. So by moving this short-circuit below that, it will still short circuit for the common case where a version is not vulnerable and on latest.For reference, here's pre-change run:
And here's post-change run:
Note that the output above is slightly confusing because it's not doing a security update only, but rather a normal update with a specified security advisory... so I think it's fine for debugging purposes that we get the information that there's no non-vulnerable version, and also that from a version-checking perspective there's no available update, regardless of vulnerability.